Fix document sorting in Elasticsearch shared storage#157
Fix document sorting in Elasticsearch shared storage#157juanpabloxk wants to merge 1 commit intomainfrom
Conversation
WalkthroughChanged Elasticsearch read query sorting in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Storage as SharedStorage
participant ES as Elasticsearch
Client->>Storage: read(query)
Storage->>ES: search(sort: inserted_at DESC)
ES-->>Storage: results (newest first)
Storage-->>Client: return results
note over Storage,ES: Sorting changed from ASC to DESC on inserted_at
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
Pull Request Test Coverage Report for Build 18114611470Details
💛 - Coveralls |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| } | ||
| }, | ||
| sort: [{ inserted_at: { order: "asc" } }] | ||
| sort: [{ inserted_at: { order: "desc" } }] |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify test coverage and confirm the intended processing order.
The change from ascending to descending order means the most recently inserted document will now be processed first (LIFO) instead of the oldest (FIFO). This is a significant behavioral change that could impact consumers of this API.
Please verify:
- Test coverage exists for this sorting behavior
- No consumers depend on the previous FIFO ordering
- This descending order aligns with the intended business requirements
Run the following script to check for related tests and usage:
🏁 Script executed:
#!/bin/bash
# Description: Find tests and usages related to Elasticsearch shared storage ordering
# Search for test files related to Elasticsearch shared storage
echo "=== Test files for Elasticsearch shared storage ==="
fd -e rb -g '*elasticsearch*' -g '*spec*' -g '*test*'
# Search for assertions or expectations about ordering/sorting
echo -e "\n=== Test assertions about sorting or ordering ==="
rg -n -C3 --type=ruby 'inserted_at|sort|order|first|FIFO|LIFO' -g '*spec*' -g '*test*'
# Search for usages of the read method from Elasticsearch class
echo -e "\n=== Usages of SharedStorage::Elasticsearch read method ==="
rg -n -C3 --type=ruby 'SharedStorage::Elasticsearch|Elasticsearch.*\.read'Length of output: 5424
Add tests covering descending inserted_at sorting behavior
Currently only the single-hit case is tested; add specs in spec/bas/shared_storage/elasticsearch_spec.rb verifying multiple results are returned newest-first, and confirm no downstream consumers depend on FIFO ordering or conflict with business requirements.
2 participants
Summary by CodeRabbit